Solid implementation. The concurrency model is sound under the existing lock discipline. Two low-severity observations below.

The putSubscriptions return value is intentionally discarded in retryRefused because retry=true means refuse() never adds PendingValues. A one-line comment at the call site would help future readers. The tests are thorough — three distinct retry scenarios plus integration with the lifecycle and communication-loss suites.

This review was generated by an AI agent, Hermès on behalf of @gnodet.

Thanks Serdar, this is a nice improvement and it follows the proposal in the JIRA. The retry is bounded per attempt, runs on the component executor, stops cleanly on consumer stop, dispose and reconnect, and a failed attempt leaves no monitored item behind in Milo's subscription. The tests and docs are in place.

Point A inline (the retry blocking under lock) is the one I would like settled before merge; the others are small.

Two questions, fine as follow-ups:

1. A node that is never created, for example a typo in the node id, is retried every 10 s for the life of the connection, with only DEBUG after the first WARN. Would it be worth exposing the refused nodes, for example in a health check or the dev console?
2. A retry that keeps getting a bad service result (for example Bad_TooManyOperations or Bad_ServiceUnsupported) is also retried forever at DEBUG and never escalated. Is that intended?

Claude Code on behalf of davsclaus. This is a rules-and-conventions review, not a replacement for static analysis or specialised review tools. This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.

Following up on my earlier review — both previous findings are fully addressed by this commit.

gnodet-bot finding #1 — retryJob lock safety: ✅ Addressed. cancelRetry() is now called under lock in all four paths (SubscriptionManager.dispose(), handleConnectionFailure(), performAndEvalConnect() on both old-connection and new-connection failure). Connected.dispose() runs after the lock is released and no longer touches retryJob. The race is eliminated.

gnodet-bot finding #2 — stale handle in refused: ✅ Addressed. deactivate() now calls this.refused.remove(clientHandle) before removing from goodSubscriptions, and deactivate is called under lock. Any handle that escapes one cycle is harmlessly filtered by the null-subscription check in retryRefused.

davsclaus finding B: ✅ Same as finding #1 — resolved.davsclaus finding C: ✅ CommunicationLossTest now uses await().during(12s).atMost(15s) instead of a bare sleep.davsclaus finding D: ✅ WARN message now reads "...trying again every {} ms: {}".

davsclaus finding A (lock held during I/O): The Javadoc on retryRefused clearly documents this as intentional, mirroring the established pattern for consumer registration. The wait is bounded by session liveness and request timeout. Accepted.

Previous findings are all addressed. LGTM.

This review was generated by an AI agent, Hermès on behalf of @gnodet.

Thanks Serdar, the points of my earlier review are addressed:

• A (retry under lock): answered. The numbers from the node-opcua run and the new javadoc of retryRefused explain why the wait is bounded, and I agree that splitting putSubscriptions into prepare/call/apply is not worth the new interleavings. Yes please to the separate JIRA for the producers not taking lock. gnodet's draft #27956 (CAMEL-25694) changes the same four producer methods (call, write, readValues, browse), so please link the two.
• B: fixed. cancelRetry() is called under lock everywhere, and Connected.dispose() no longer touches retryJob. This also covers gnodet-bot's volatile comment.
• C: fixed with during, which now also checks that the refused node is not reported during the outage.
• D: fixed, the WARN says the item is tried again and how often.
• Questions 1 and 2: answered. A dev console that lists the refused nodes per connection sounds right; yes please to the JIRA.

Two notes, not blocking:

1. On gnodet-bot's second comment (an orphaned handle in connected.refused): I do not think it can happen. unregisterItems removes the handle from subscriptions, and deactivate removes it from refused, under the same lock. The null check in retryRefused is only defensive. Because of that check, stoppedConsumerIsNotCreatedAgain would also pass without the refused.remove in deactivate. That is fine for a test that pins the behaviour.
2. I checked the Milo 1.1.8 contract. OpcUaSubscription.createMonitoredItems does not throw on a lost connection or a reset subscription. It returns a bad service result for each item (Bad_InvalidState when the subscription has no server state), so a failed attempt removes its items again and leaves nothing behind. Milo splits a Bad_TooManyOperations call into one item per call, and Bad_TooManyMonitoredItems comes back as an operation result, so it is retried as the JIRA asks.

There is no code overlap with #27956, which only touches the producer methods.

LGTM.

Claude Code on behalf of davsclaus. This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.

TL;DR

Thanks Serdar, the retry is nicely contained. It runs on the executors of the connection and is cancelled under lock wherever a connection is replaced or disposed. It never rebuilds the connection, and the tests cover a late node, a late namespace, a stopped consumer and an outage. No blockers from this review; one new race worth a look.

Blockers

• None

Other findings: 2 (1 non-blocking, 1 suggestion)

1. A refusal still being delivered can follow the first value of an item that a retry created in the meantime (non-blocking)
Subscription.refuse() (SubscriptionManager.java:300-301) sets refused only when the PendingValue is delivered. Those deliveries run outside lock and run the route for one node after another (:1111 recreate, :1634 registerItems). A newly refused node joins a retry chain that may already be running (:508), so its first attempt can come well before 10 s. If that attempt creates node N (:451 clears refused) before N's pending refusal is delivered, the route receives N's first Good value and then the stale refusal. refused then stays true, so report() (:313) drops Bad_NoCommunication for N for the rest of the connection. It needs a consumer with several nodes, a slow route and a node that appears right after its refusal, so it is unlikely, but it comes with the same-connection retry. The reconnect path is safe: its deliveries and the retry timer share the single reconnect thread. Could we record the refusal under lock and deliver it only while it still holds? For example:

// Connected.refuse(), first refusal: under lock, a later successful creation clears it (:451)
s.refused = true;
pendingValues.add(new PendingValue(s, this.generation, new DataValue(status)));

// Subscription
synchronized void refuse(final long generation, final DataValue status) {
    if (this.refused) { // not created by a retry in the meantime
        deliver(generation, status);
    }
}

2. Test time (suggestion)
The interval is fixed at 10 s (MiloClientConnection passes 10_000), so the new tests wait for one or more full intervals (MonitoredItemRetryTest.java:193 15 s, CommunicationLossTest.java:240 12 s). Once CAMEL-25873 lets reconnectTimeout set the interval, they could use a shorter one.

Housekeeping: B, C, D and the retryJob visibility note look addressed by c928243. The orphan-handle note looks moot, because deactivate() (:553) drops the handle under the same lock that removes it from subscriptions.

Reviewed with Claude Code on behalf of oscerd. This review was generated by an AI agent and may contain inaccuracies; please verify all suggestions before applying.

Thanks Serdar. I traced c68d3a1: the node is marked refused under lock once putSubscriptions succeeds, refused is cleared under the same lock when the item is created, and Subscription.refuse() delivers only while the flag holds. A refusal overtaken by a retry or a new connection is dropped. One already being delivered still comes before the first value, because the value goes through the same monitor. A call that fails leaves the nodes to the loss report. This covers Andrea's finding 1. refusalOvertakenByTheNextAttemptIsNotDelivered pins it, and the ordering it relies on holds because the consumer and registerItems both keep the node order. Thanks also for opening CAMEL-25912 and CAMEL-25913.

LGTM, once CI is green.

This review was generated by an AI agent (Claude Code on behalf of davsclaus) and may contain inaccuracies. Please verify all suggestions before applying.